Test the simulation mode code. #5712

wxtim · 2023-08-30T09:24:27Z

Pre-requisite work for Skip mode

Refactor simulation mode functions.
Improve documentation of sim mode settings.
Ensure 100% test coverage of sim mode.

On an initial self-review I noticed that configure_sim_modes might as well be in the simulation module. I've moved it, but not squashed that commit down, because that way reviewers can see how I refactored it in a separate commit to the one I move it in.

Check List

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
Tests are included (or explain why tests are not needed).
CHANGES.md entry included if this is a change that can affect users
Cylc-Doc pull request opened if required at Updated sim mode docs cylc-doc#641.
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

wxtim · 2023-08-30T11:59:06Z

Changed to draft because a final sanity check/attempt to write an example for the docs showed up at least 2 exciting bugs.

For the record these bugs are as follows:

Traceback when trying to resubmit simulated task:

Using example:

[scheduling]
    initial cycle point = 2359
    [[graph]]
        R1 = get_observations

[runtime]
    [[get_observations]]
        execution retry delays = PT2S
        [[[simulation]]]
            fail cycle points = all

fails with traceback after the method _simulation_submit_task_jobs sets platform = {'name': 'SIMULATION'}, thereby deleting all other settings!

Resubmit task hangs in waiting state:

Using the same example but fixing the first bug. Resubmit hangs forever because the _simulation_submit_task_jobs keep gives the simulated second submission a status of waiting before calling process_message, which then fails to do anything because the task is in a waiting state.

changes.d/5712.fix.md

oliver-sanders

Approach looks good, I like having all the simulation stuff in once place!

Some minor comments.

cylc/flow/task_job_mgr.py

tests/functional/modes/03-simulation.t

tests/integration/test_simulation.py

cylc/flow/simulation.py

wxtim · 2023-09-12T10:10:04Z

cylc/flow/simulation.py

+            itask.summary['started_time'] = now
+        timeout = (
+            itask.summary['started_time'] +
+            itask.tdef.rtconfig['simulation']['simulated run length']


Not a real config item, added to the rtconfig['simulation'] dictionary at line 54.

cylc/flow/task_events_mgr.py

wxtim · 2023-09-12T10:12:03Z

cylc/flow/task_events_mgr.py

-                itask.state(TASK_STATUS_WAITING)
-                and
+            itask.state(TASK_STATUS_WAITING)
+            and itask.tdef.run_mode == 'live'   # Polling in live mode only.


The code within this section is designed to avoid problems with polling - but there will be no polling in simulation mode.

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

@MetRonnie

* Remove unecessary rtc['platform'] (@MetRonnie spotted this).

* 'test.sim_mode' of github.com:wxtim/cylc: Update tests/functional/modes/03-simulation.t

* upstream/master: speed up import time (cylc#5770) graphql: remove extraLogs field (cylc#5672) give a more useful error message if remote init fails (cylc#5720) clarify SLURM job handler docs (cylc#5748)

tests/unit/test_config.py

tests/unit/test_simulation.py

cylc/flow/simulation.py

tests/unit/test_simulation.py

wxtim · 2023-10-18T11:16:34Z

@oliver-sanders - accidentally re-tagged you for review. Happy for you to re-approve

cylc/flow/task_job_mgr.py

MetRonnie · 2023-10-19T14:35:25Z

While you're here Tim, could you check that restarting a workflow that has been running in simulation mode will restart it in simulation mode without having to specify it? I saw it restart in live mode, would be good to double check.

Also the reverse. Oliver said trying to restart in simulation mode when it had been running in live mode merely restarted in live mode.

wxtim · 2023-10-20T09:25:01Z

check that restarting a workflow that has been running in simulation mode will restart it in simulation mode without having to specify it?

Yes, if you restart you do not have to specify mode to get the same mode.

Oliver said trying to restart in simulation mode when it had been running in live mode merely restarted in live mode.

When I tried this on master it looks like it restarts in simulation mode (on master & my branch). Ick.

That's a bug: I've opened an issue at #5781

hjoliver · 2023-10-24T22:01:02Z

This has two approvals, but I'm not sure if all questions addressed yet? If so, let's merge it.

wxtim · 2023-10-25T08:51:53Z

This has two approvals, but I'm not sure if all questions addressed yet? If so, let's merge it.

I think that I have addressed all questions. There is a comment from Ronnie about restart where the response was to create a new issue - the bit about what mode a restarted workflow uses.

Agreed, but I think @MetRonnie or @oliver-sanders should press the button here.

MetRonnie

Pending resolution of Oliver's comment #5712 (comment)

wxtim requested review from hjoliver and oliver-sanders and removed request for hjoliver August 30, 2023 09:24

wxtim self-assigned this Aug 30, 2023

wxtim added this to the cylc-8.3.0 milestone Aug 30, 2023

wxtim added doc Documentation could be better Not exactly a bug, but not ideal. labels Aug 30, 2023

wxtim force-pushed the test.sim_mode branch 3 times, most recently from ce9c70c to d5ec93a Compare August 30, 2023 09:44

wxtim marked this pull request as draft August 30, 2023 11:57

wxtim marked this pull request as ready for review August 31, 2023 12:31

wxtim mentioned this pull request Aug 31, 2023

Updated sim mode docs cylc/cylc-doc#641

Merged

1 task

oliver-sanders reviewed Aug 31, 2023

View reviewed changes

changes.d/5712.fix.md Outdated Show resolved Hide resolved

wxtim requested a review from oliver-sanders August 31, 2023 15:32

oliver-sanders reviewed Sep 4, 2023

View reviewed changes

wxtim mentioned this pull request Sep 6, 2023

Feature: Sim mode at runtime #5721

Merged

8 tasks

wxtim requested a review from oliver-sanders September 6, 2023 15:28

oliver-sanders reviewed Sep 11, 2023

View reviewed changes

cylc/flow/simulation.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Sep 11, 2023

View reviewed changes

cylc/flow/simulation.py Outdated Show resolved Hide resolved

wxtim force-pushed the test.sim_mode branch from 6a4dd3b to 74a6496 Compare September 12, 2023 10:08

wxtim commented Sep 12, 2023

View reviewed changes

cylc/flow/task_events_mgr.py Outdated Show resolved Hide resolved

wxtim commented Sep 12, 2023

View reviewed changes

wxtim requested a review from oliver-sanders September 12, 2023 10:13

wxtim marked this pull request as draft October 5, 2023 07:59

wxtim marked this pull request as ready for review October 9, 2023 14:50

wxtim marked this pull request as draft October 9, 2023 14:50

wxtim and others added 5 commits October 18, 2023 08:42

Update tests/functional/modes/03-simulation.t

4370774

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

* Only import typing objects in TYPE_CHECKING mode.

e6f9af1

* Remove unecessary rtc['platform'] (@MetRonnie spotted this).

Merge branch 'test.sim_mode' of github.com:wxtim/cylc into test.sim_mode

d25a6db

* 'test.sim_mode' of github.com:wxtim/cylc: Update tests/functional/modes/03-simulation.t

remove sub retry delays bodge

9adfb37

Merge remote-tracking branch 'upstream/master' into test.sim_mode

12ba4c9

* upstream/master: speed up import time (cylc#5770) graphql: remove extraLogs field (cylc#5672) give a more useful error message if remote init fails (cylc#5720) clarify SLURM job handler docs (cylc#5748)

wxtim marked this pull request as draft October 18, 2023 10:02

wxtim marked this pull request as ready for review October 18, 2023 10:17

wxtim requested a review from MetRonnie October 18, 2023 10:18

MetRonnie reviewed Oct 18, 2023

View reviewed changes

wxtim requested review from MetRonnie and oliver-sanders October 18, 2023 11:16

MetRonnie approved these changes Oct 18, 2023

View reviewed changes

MetRonnie removed the doc Documentation label Oct 18, 2023

oliver-sanders reviewed Oct 18, 2023

View reviewed changes

cylc/flow/task_job_mgr.py Show resolved Hide resolved

wxtim mentioned this pull request Oct 20, 2023

Run mode not conserved on restart #5781

Closed

wxtim force-pushed the test.sim_mode branch 2 times, most recently from 409742f to 5241558 Compare October 24, 2023 08:34

fix merge mistake

acf659f

wxtim force-pushed the test.sim_mode branch from 5241558 to acf659f Compare October 24, 2023 08:36

MetRonnie approved these changes Oct 25, 2023

View reviewed changes

oliver-sanders approved these changes Oct 26, 2023

View reviewed changes

oliver-sanders merged commit 8606f93 into cylc:master Oct 26, 2023
23 of 25 checks passed

wxtim deleted the test.sim_mode branch October 26, 2023 09:39

wxtim added a commit to wxtim/cylc that referenced this pull request Jan 16, 2024

backport two fixes from cylc#5712

67e417b

wxtim mentioned this pull request Jan 17, 2024

backport fix from #5712 #5927

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test the simulation mode code. #5712

Test the simulation mode code. #5712

wxtim commented Aug 30, 2023 •

edited

Loading

wxtim commented Aug 30, 2023 •

edited

Loading

oliver-sanders left a comment

wxtim Sep 12, 2023

wxtim Sep 12, 2023

wxtim commented Oct 18, 2023

MetRonnie commented Oct 19, 2023 •

edited

Loading

wxtim commented Oct 20, 2023 •

edited

Loading

hjoliver commented Oct 24, 2023

wxtim commented Oct 25, 2023 •

edited

Loading

MetRonnie left a comment •

edited

Loading

Test the simulation mode code. #5712

Test the simulation mode code. #5712

Conversation

wxtim commented Aug 30, 2023 • edited Loading

wxtim commented Aug 30, 2023 • edited Loading

Traceback when trying to resubmit simulated task:

Resubmit task hangs in waiting state:

oliver-sanders left a comment

Choose a reason for hiding this comment

wxtim Sep 12, 2023

Choose a reason for hiding this comment

wxtim Sep 12, 2023

Choose a reason for hiding this comment

wxtim commented Oct 18, 2023

MetRonnie commented Oct 19, 2023 • edited Loading

wxtim commented Oct 20, 2023 • edited Loading

hjoliver commented Oct 24, 2023

wxtim commented Oct 25, 2023 • edited Loading

MetRonnie left a comment • edited Loading

Choose a reason for hiding this comment

wxtim commented Aug 30, 2023 •

edited

Loading

wxtim commented Aug 30, 2023 •

edited

Loading

MetRonnie commented Oct 19, 2023 •

edited

Loading

wxtim commented Oct 20, 2023 •

edited

Loading

wxtim commented Oct 25, 2023 •

edited

Loading

MetRonnie left a comment •

edited

Loading